Skip to content

[Clippy] Use symbols intended for arithmetic_side_effects #115415

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 1, 2023
Merged

[Clippy] Use symbols intended for arithmetic_side_effects #115415

merged 1 commit into from
Sep 1, 2023

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Aug 31, 2023 •
edited by flip1995
Loading

#115177 added the symbols for arithmetic_side_effects and now this PR actually uses them to prevent an eventual removal. All because #115183 was recently merged and next Clippy update will probably take some time to happen.

Fixes rust-lang/rust-clippy#11392

@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2023

r? @cuviper

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 31, 2023

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@c410-f3r
Copy link
Contributor Author

@rustbot labels -T-libs +T-compiler

r? compiler

Is there a better team?

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 31, 2023
@rustbot rustbot assigned oli-obk and unassigned cuviper Aug 31, 2023
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@c410-f3r
Copy link
Contributor Author 

- *****$DIR*****/arithmetic_side_effects.rs:304:5
+ *****tests/ui*****/arithmetic_side_effects.rs:304:5

For some reason the original $DIR should now be tests/ui, which is strange.

@flip1995
Copy link
Member

Next Clippy update is in 1 week. So this PR could only add the diag items and then the logic could be changed in a Clippy PR. But that would take another 2 weeks to get in.

I'm good with making an exception here and making functional Clippy changes in the Rust repo in this case.

flip1995
flip1995 approved these changes Aug 31, 2023
View reviewed changes
Copy link
Member

@flip1995 flip1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The impl LGTM, but is there a Clippy issue associated with this, that could be linked in this PR?

@flip1995
Copy link
Member

r? @flip1995

@rustbot rustbot assigned flip1995 and unassigned oli-obk Aug 31, 2023
@c410-f3r
Copy link
Contributor Author

Hehehe, sorry for the mess @flip1995.

The related Clippy issue has been attached.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@flip1995 flip1995 mentioned this pull request Sep 1, 2023
Closed
flip1995
flip1995 reviewed Sep 1, 2023
View reviewed changes
@flip1995
Copy link
Member

flip1995 commented Sep 1, 2023

That seemed to have worked. I rebased and squashed the blessing commits.

Thanks!

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 1, 2023

📌 Commit 0164f7e has been approved by flip1995

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 1, 2023
@flip1995
Copy link
Member

flip1995 commented Sep 1, 2023

@bors rollup

@bors
Copy link
Collaborator

bors commented Sep 1, 2023

⌛ Testing commit 0164f7e with merge d6b4d35...

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Sep 1, 2023

Nice! Thank you very much @flip1995

@bors
Copy link
Collaborator

bors commented Sep 1, 2023

☀️ Test successful - checks-actions
Approved by: flip1995
Pushing d6b4d35 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 1, 2023
@bors bors merged commit d6b4d35 into rust-lang:master Sep 1, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 1, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d6b4d35): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.8% [2.8%, 2.8%] 1
Improvements ✅
(primary)		 -1.5% [-1.9%, -1.1%] 3
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) -1.5% [-1.9%, -1.1%] 3

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 - - 0
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 -2.9% [-3.0%, -2.8%] 3
All ❌✅ (primary) - - 0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.878s -> 629.909s (-0.31%)
Artifact size: 316.40 MiB -> 316.39 MiB (-0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flip1995 flip1995 flip1995 approved these changes

Assignees

@flip1995 flip1995

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.74.0
Development

Successfully merging this pull request may close these issues.

[arithmetic_side_effects] Division by NonZeroU* cannot panic
8 participants
@c410-f3r @rustbot @rust-log-analyzer @flip1995 @bors @rust-timer @cuviper @oli-obk